-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[GS-7591] Add new Step Slider component #250
Conversation
🦋 Changeset detectedLatest commit: b1e4db6 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
…l configs. Change babel file externsion
…l configs. Change babel file externsion
771d85a
to
aff206b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Outstanding work!
I owe you a review locally.
I was able to do due diligence and managed to leave feedback. Thanks @JBezerra!
packages/provider-elasticsearch/src/example-types/filters/step.test.js
Outdated
Show resolved
Hide resolved
{/** This padding exists because of contexture native margins. | ||
* When the slider thumb is encapsulated on the filter UI, | ||
* part of it gets cut if no padding is applied. */} | ||
<Box marginTop="6px" paddingX="11px"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A request for em/rem over px
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about properly discussing that before moving forward with this change? My reason for that consideration is because we are using pixels as our measure everywhere on contexture
and spark
.
Doing this change in isolation wouldn't make sense if we are not willing to set that as our new standard from here and beyond. Other than that, that are other considerations we should do before starting using rem
on a codebase founded in pixels.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about em
? What are the repercussions of using that?
I might be missing something, but I don't see drawbacks specifically with em(?).
Also happy to discuss as a team. In fact, that could be more effective to tackle as a group,
v.s a siloed PR. 👍🏾
8b9ab9f
to
e46381c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Verified locally, great work champ!! 🏆
d87b453
to
16e9bbf
Compare
b7c9864
to
b2e80ce
Compare
0a7c29d
to
8dbf2ef
Compare
Coverage after merging GS-7591/step-slider into main will be
Coverage Report
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GTG
GS-7591
Before you start
A few things related to tooling were installed and configured so we can have better DX/reliability going forward:
In order to have better understanding how to link the changes of this PR to your local spark, please read the docs.
TL;DR Version
On Spark
On Contexture
On WebApp
Population Count
filter to your Spending page.Demo
2024-08-16.14-27-47.mp4
Context
As a Product Manager, I would like to have a more intuitive way for users to filter using Population Count in spending. Currently it is a range filter where users have to enter in a range to search. It would be nice if we could have pre-set population ranges for users to select and filter.
Start with preset ranges, below are the suggestions:
Acceptance Criterias: